Skip to content

fix(pr-verify): accept any font fc-list reports for U+23F8 - #13720

Merged
kyle-sexton merged 1 commit into
mainfrom
fix/12232-capture-font-check
Nov 6, 2026
Merged

kyle-sexton merged 1 commit into
mainfrom
fix/12232-capture-font-check

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

Refs: #12232

Summary

Every live PR capture fails at "Install the screenshot font" since #13707: capture run 42230112786 annotated no installed font has U+23F8. The check matched the family name Noto Sans Symbols 2; the runner's fc-list prints a different family string for the same file (the probe run printed only the file, NotoSansSymbols2-Regular.ttf). The step fails before the proxy starts, so no key credit is spent, but no capture completes.

Fix

  • The check passes when fc-list ':charset=23f8' family file lists any font, and prints what it found, so the next run records the family string.

Verification

  • New test: the check, run with a stubbed fc-list, passes on /usr/share/fonts/truetype/noto/NotoSansSymbols2-Regular.ttf: Noto Sans Symbols2 and fails with the error on empty output. It fails on main's text.
  • node --test .github/scripts/pr-verify/workflow.test.mjs: 76/76.

Related

Remaining

🤖 Generated with Claude Code

Capture run 42230112786 failed at "Install the screenshot font": the check
matched the family name "Noto Sans Symbols 2", which the runner's fc-list
does not print, so every capture stopped there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 6, 2026 14:16
@chatgpt-codex-connector

Copy link
Copy Markdown
Codex Review Summary: ❌ Failed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ❌ Failed 2026-11-06T14:16:56.774892Z
We couldn’t complete this review. Comment @codex review to try again.
a747e84 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

claude-security-review reviewed a747e84dc22296c6a7d7fe107a417ec15f9fef4e: read 2 of 2 changed file(s); 0 finding(s), 0 as inline comments.

Security review: no security issues found

I read every hunk of both changed files.

.github/workflows/pr-verify-capture.yml (trusted path; I read the whole change and what the step gates)

  • This changes only the "Install the screenshot font" step. Before, the font check required the exact family name Noto Sans Symbols 2. Now it passes when fc-list ':charset=23f8' family file lists any font, and it prints that output.
  • No trust control is affected. The step checks that a font is present before screenshots are taken. It is not a trusted-actor check, a token request, a kill-switch read or a merge hold. permissions:, triggers, if: conditions and isolation setup are unchanged.
  • No untrusted input reaches the shell. The script uses no ${{ }} expressions. The only new output is fc-list font paths and family names from packages installed on the runner. Printing them exposes no secret, token or key.
  • What the looser check could do: a non-Noto font that covers U+23F8 could now pass. At worst that changes how a screenshot renders. It has no leak, compromise or charge impact.

.github/scripts/pr-verify/workflow.test.mjs

  • This is a test only. It runs the step's script, minus the apt-get lines, against a stub fc-list in a temporary directory with a minimal PATH. The values written into the stub are constants in the test, so there is no injection surface.

No userConfig or plugin manifest changes. No instruction-surface deletions. No text in the PR context tries to direct the review.

Spend: none. Per the PR description, the step fails before the key proxy starts, and this change only lets captures finish again. That is normal lane spend from prepaid API credit, so it is not a hold.

critical-findings: 0

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

claude-review reviewed a747e84dc22296c6a7d7fe107a417ec15f9fef4e: read 2 of 2 changed file(s); 0 finding(s), 0 as inline comments.

Code review: PR #13720, fix(pr-verify): accept any font fc-list reports for U+23F8

No findings. I read every hunk in both changed files: .github/workflows/pr-verify-capture.yml and .github/scripts/pr-verify/workflow.test.mjs.

What changes. The "Install the screenshot font" step no longer requires the family string Noto Sans Symbols 2. It now passes when fc-list ':charset=23f8' family file lists any font, and it prints what it found so later runs record it. A new test runs the step's script with a stubbed fc-list. With a NotoSansSymbols2-Regular.ttf: Noto Sans Symbols2 line it expects exit 0. With empty output it expects exit 1 and the no installed font has U+23F8 error.

Checked:

  • Does any matching font meet the step's purpose? Yes. The driver does not check the font by name when it renders. Its only font check is the doctor report at driver.mjs#L1003-L1013, and that report never blocks a capture. So a font fontconfig reports as covering U+23F8 is what the rasterizer needs.
  • Does the new test catch a regression? Yes. On main's text, the Noto Sans Symbols2 stub fails the old *"Noto Sans Symbols 2"* match, so the first assertion fails there. The test also confirms the failure path still exits 1 with the error. Its imports (tempDir, spawnSync, writeFileSync, path) are already in the file. Stripping the apt-get lines leaves exactly the fc-list check under test.
  • Does bash -eo pipefail change anything? No. With the step's default shell, the [[ -n $fonts ]] || { …; exit 1; } line still reaches its error branch.
  • Is the step's comment still accurate? It still says the rasterizer "needs Noto Sans Symbols 2" (pr-verify-capture.yml#L423-L425). That is still the font the step installs, so it is not wrong.

Verification. The author reports node --test: 76/76. That is the author's claim and I did not confirm it; I had no shell in this run and ran nothing.

PR context. The PR refs #12232 and closes no issue, so there are no acceptance criteria to check. I found no embedded instructions in the PR context, and there were no prior findings: this is the lane's first review of this PR.

Spend. The PR changes no billing, plan, runner size or paid resource.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 6, 2026
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

The pr-verify verifier failed head a747e84dc22296c6a7d7fe107a417ec15f9fef4e. What it wrote, as data:

Summary: PR #13720 refs #12232, so it delivers only part of that issue. The code change is correct.
  The "Install the screenshot font" step in pr-verify-capture.yml now passes when `fc-list
  ':charset=23f8' family file` lists any font, prints what it found, and still exits 1 with the
  error when nothing is listed. The new workflow.test.mjs case runs the real step script with a
  stubbed fc-list. It checks both outcomes and would fail on the old family-name match. test-node
  concluded success on the judged head
  (https://github.com/melodic-software/claude-code-plugins/actions/runs/42232520350/job/127942167657).
  No guard is weakened, because the charset query itself still requires a font that has U+23F8. The
  block is the PR body. For a `refs` PR the body must name what remains of the issue for later work,
  and this one does not. It only points at "#12232 Phase 6", #13707 and #13712. It never says that
  done-when 4 (one in-repo PR showing a judged capture with count above 0) is still open.
- PR body (#13720): add a Remaining section that names what is still open on #12232 after this fix.
  At minimum that is done-when 4: one in-repo PR that changes a UI surface and shows a judged
  capture with count above 0. Also list the Q7/Q9 hardening follow-ups if they belong to this phase.
  A `Refs:` PR must name what remains.

This pull request leaves the merge queue, and ci-status holds it out while it carries pr-verify-failed. A pr-verify pass removes the label, at a new commit or at this same head: once a trusted person edits the body, and only trusted people have edited it since the fail, this head is judged again, so a fail on the body or the issues it links needs only the body fixed, no new commit.

@github-actions github-actions Bot added pr-verify-failed pr-verify failed this head; a pass at a new commit, or after a trusted body edit, removes it. and removed pr-verify-failed pr-verify failed this head; a pass at a new commit, or after a trusted body edit, removes it. labels Nov 6, 2026
Merged via the queue into main with commit 6d9aee8 Nov 6, 2026
75 of 87 checks passed
@kyle-sexton
kyle-sexton deleted the fix/12232-capture-font-check branch November 6, 2026 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant